Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grand Christmas Patch #479

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Grand Christmas Patch #479

merged 1 commit into from
Dec 18, 2020

Conversation

SylwiaBrant
Copy link

Fixed helm not creating role permissions #424
Added ability to use custom k8s cluster domain #461
Fixed propagation of JENKINS_OPTS to seed-job agent #462
Fixed updating last backup #421
Also updated plugins.

Comment on lines 12 to 32
var jenkins = v1alpha2.Jenkins{
Spec: v1alpha2.JenkinsSpec{
Master: v1alpha2.JenkinsMaster{
Containers: []v1alpha2.Container{
{
Env: []corev1.EnvVar{
},
ReadinessProbe: &corev1.Probe{
Handler: corev1.Handler{
HTTPGet: &corev1.HTTPGetAction{
},
},
},
LivenessProbe: &corev1.Probe{
Handler: corev1.Handler{
HTTPGet: &corev1.HTTPGetAction{
},
},
},
},
},
},
},
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please run go fmt in this folder, i think there are additional unnecessary indents

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eagle eye :D

Comment on lines +293 to +306
jenkinsOptsEnv := envs[key]
jenkinsOptsWithDashes := jenkinsOptsEnv.Value
if len(jenkinsOptsWithDashes) == 0 {
return nil
}

jenkinsOptsWithEqOperators := strings.Split(jenkinsOptsWithDashes, " ")

for _, vx := range jenkinsOptsWithEqOperators {
opt := strings.Split(vx, "=")
jenkinsOpts[strings.ReplaceAll(opt[0], "--", "")] = opt[1]
}

return jenkinsOpts

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see some magic here. are you sure that cannot be simpler?

Comment on lines 76 to 80
} else if err != nil {
return "", nil
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use else if, if you return something in first condition

This code looks good, but I think it can be simpler

runningInCluster, err := isRunningInCluster();
if !runningInCluster {
	return kubernetesClusterDomain, nil
}
 
if err != nil {
	return "", nil
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's up to you, which version you choose

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you can see it's not my part of the code, but you're right. It needs a refactor.

Comment on lines +404 to +408

suffix := ""
if prefix, ok := resources.GetJenkinsOpts(*jenkins)["prefix"]; ok {
suffix = prefix
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this piece of code in some places, maybe it's good idea to export it to dedicated function? (DRY)

Fixed helm not creating role permissions #424
Added ability to use custom k8s cluster domain #461
Fixed propagation of JENKINS_OPTS to seed-job agent #462
Fixed updating last backup #421
@SylwiaBrant SylwiaBrant requested a review from tumevoiz December 15, 2020 09:20
@SylwiaBrant SylwiaBrant requested a review from tumevoiz December 16, 2020 10:33
@tomaszsek tomaszsek merged commit d6b10c6 into jenkinsci:master Dec 18, 2020
@jam01
Copy link

jam01 commented Dec 22, 2020

Happy holidays, all. I've been trying to keep track of the state of the operator projects, sorry to drop in here but was wondering if we're to see a 0.4.1 or 0.5 release soon?

@tomaszsek @SylwiaBrant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants